refactor(backups): drop node-7z and 7zip-bin by moving backups to gzipped tar - #274

You are right about the restore path, and thank you for tracing it all the way to the discard(replacedPath) call rather than stopping at the missing handler. Fixed and pushed.

The blocking one

I took strict: true rather than the warn handler, because the handler would upgrade only TAR_ENTRY_ERROR while strict also covers TAR_ENTRY_INVALID, a corrupt entry header whose entry is dropped every bit as quietly and would truncate a restore the same way.

Everything else strict makes fatal is already refused before it can happen, which is the part I wanted to be sure of before picking it. TAR_ENTRY_UNSUPPORTED never fires: the filter runs in the parser, at the point the header is read, so an entry of a type we refuse is ignored and never reaches [UNSUPPORTED]. TAR_ENTRY_INFO for a stripped absolute path cannot be reached either, since validateArchive walks the table of contents and refuses that archive by name before a byte is written. TAR_BAD_ARCHIVE and TAR_ABORT were already errors regardless, Unpack.warn marks them unrecoverable. So the only behaviour that actually changes is the one that had to. Strict has one more thing going for it in the scenario you described: it stops at the first entry that fails instead of running the rest of a multi-GB archive into a disk that has no room left for it.

The test builds an archive whose second entry cannot be written. a is a plain file, and a/b then asks for a to be a folder, so the write fails for that one entry and the unpacker moves on to the next exactly as it does when the disk fills. I went that way instead of an unwritable destination for two reasons. runExtraction unpacks into its own mkdtemp folder, which a test cannot make unwritable from the outside, and a permission based test quietly stops testing anything when it runs as root. Two assertions hang off it: runExtraction rejects and leaves the destination empty, and that same archive driven through restoreInstallationBackup, on a real filesystem with an extractor shaped the way extractWorker.ts shapes it, comes back extract-failed with the installation still holding its file.

Taking strict: true back out fails three tests. The two new ones, plus the existing "writes nothing outside the destination for a tar.gz climbing out" case, which is now stricter than it was. Its .. and absolute entries used to be skipped with the extraction resolving anyway, and they now fail it, so that assertion moved from a plain await to a rejection.

The two smaller ones

extractZip now keeps the entry's read and write streams in the closure and unpipes and destroys them in finish when it settles with a failure, so both ends are stopped before runExtraction's finally gets to the temporary folder. Same shape as extractTarGz eleven lines down. This one has no test, deliberately: the symptom is the Windows EBUSY, and on Linux the removal succeeds whether or not the handle is still open, so a test would pass with or without the fix and say nothing.

A failed tar.create now takes the partial archive away before rethrowing. The test drives a compression that fails partway through, using a source file the safety walk can stat but tar cannot read, and asserts the backups folder is empty afterwards. It carries the same skip guard the account store tests use, since neither Windows nor root would honour the permission. Dropping the removeSync fails it.

Items 3 and 4 and the minor list are untouched here.

Gates

npm ci clean. npm run typecheck, npm run lint:ci at 0 errors and the same 15 pre-existing warnings, and npm run format:check all pass. npm run test:coverage is 137 files, 1646 passing and 2 skipped, at 92.61 statements, 89.87 branches, 92.16 functions and 94.04 lines, every floor clear. Linux only, so the zip teardown stays unexercised for the reason it exists.

Second pass, working through the rest of the review. Three of the remaining points are fixed, two are answered rather than changed, and one is fixed differently from the way you suggested. Reasoning for each below.

3. The size the reader will not accept

Fixed, and I think this one was closer to the blocking finding than its position in the list suggests. Refusing an oversized source in runCompression is the substance of it, so the archive is never written.

What makes it worse than "a restore that fails with a generic error" is the ordering in makeInstallationBackup: pruneOldestBackups runs before the archiver, so taking a backup deletes the oldest one to make room and only then writes the new one. An installation over the cap therefore evicted a restorable backup and replaced it with a file that could never be restored, once per attempt, until nothing restorable was left. The archive is now refused before tar opens anything.

Two things I did not do. The prune-before-write ordering itself is untouched: pruning first is what keeps the folder from holding one more archive than the limit at its peak, and changing it is a backup-semantics decision that wants its own change rather than a line in this one. And the message does not reach the UI, because it cannot: compressWorker.ts maps every error to the constant "Compression failed" rather than passing error.message the way extractWorker.ts does, and COMPRESS_ON_PATH returns a boolean, so the renderer sees compress-failed and nothing more. A distinct, renderable reason means a new failure reason in the domain, a locale key in fourteen files and a branch in describeBackupFailure, which is a feature rather than the one-liner, and I would rather propose it separately than smuggle it in here.

The test uses a sparse file: 3 GiB by every stat, no blocks on disk, so it costs nothing and skips on Windows where truncate is not sparse. Removing the check fails it, and takes four seconds doing it, which is the run the guard now avoids.

4. Unix modes on a legacy zip restore

Taken as the second option you offered, documented rather than implemented, and I want to say why rather than just point at the comment.

On Linux nothing preserves modes today, including the tar path: startExtract in TaskManagerContext.tsx runs changePerms([outputPath], 0o755) after every extraction, restores included, which overwrites whatever tar restored. So the asymmetry is macOS only, on a platform where the game cannot be launched, for a folder holding saves and settings with nothing executable in it.

Against that, restoring a mode read out of an archive written by a tool the launcher no longer ships is not free. A zip made on Windows carries 0 in the upper half of externalFileAttributes, and chmodding a save file to 0 would be a worse bug than the one being fixed, so the honest version needs a made-by check, a mask and a zero guard. That is more code, and more risk, than the thing it buys. The comment now says the omission is deliberate and gives these reasons, so the next reader does not have to rediscover them.

Progress fidelity

Not changed, and I think it should stay as it is. The accurate fix is byte accurate, and the only clean way to get there is a counting stream interposed in the pack pipeline, since onWriteEntry fires at header time by construction. Listening on the entry stream instead would count the 512 byte header and the padding as content, which trades one inaccuracy for another and touches the flow of the pipeline that writes the archive. Weighed against a bar that reaches 99 early for an installation dominated by one large save, with monotonicity, dedup and the single terminal 100 all still holding, that is not a trade worth making inside this change. Happy to be overruled if the jump bothers you more than it bothers me.

The fixture comment

You were right, and the fixture was hiding more than a wrong sentence. yauzl's validateFileName refuses /^[a-zA-Z]:/ exactly as it refuses a leading slash and a .. segment, so all three names in hostile-backup.zip stop the read and none of them reaches isSafeArchiveEntry. Comment corrected.

I also took your suggestion for a name yauzl accepts but the launcher refuses. A NUL byte in the middle of a name is one: validateFileName says nothing about it, so unsafe-name-backup.zip reaches the launcher's own gate and is refused there. Worth having, because I checked what the suite did without it: deleting !isSafeArchiveEntry(entry.fileName) from validateZipArchive failed exactly zero of the 1649 other tests. It now fails one.

The rebuild also produced a one byte change in oversized-declared-icon.zip, inside its DEFLATE stream, from a different zlib build rather than from anything in this change. I reverted that file rather than commit the churn.

The stale sweep

Done, plus three the list did not name: the worker timeout comment in pathsHandlers.ts about a task "still holding a 7-Zip child", isTarGzName in validation.ts explaining itself in terms of what 7-Zip cannot read, and DEFAULT_COMPRESSION_LEVEL in defaults.ts described as a 7-Zip level. pathPolicy.ts and the archiveConcurrency rationale are updated as you asked.

ADR 0001 I handled differently, and tell me if you disagree. It is dated, its status is proposed, it says outright that every number in it was measured on 2026-08-16, and Option A's cost paragraph argues partly from the 7za l -slt reader. Editing those lines would put today's tree into an argument that was put to the deciders with the numbers of the day, and would leave every other stale figure in there looking current by association. It gets a dated note instead, saying what no longer holds and pointing at this PR, with the measurements left as they were taken.

Gates

npm run typecheck, npm run lint:ci at 0 errors and the same 15 pre-existing warnings, and npm run format:check all pass. npm run test:coverage is 137 files, 1648 passing and 2 skipped, at 92.70 statements, 89.95 branches, 92.16 functions and 94.13 lines, every floor clear. Both new tests were checked by removing the guard they cover: the size refusal and the name gate each fail exactly one test when taken out.

Went with the second option, so a hard linked source now produces a self contained backup instead of a refusal.

The reason comes down to who ends up holding these. A player does not have to have made a hard link to have one. Deduplicating filesystems hand them out on their own, and so does anything that copies a mod folder with link semantics rather than by reading and rewriting the bytes. Refusing the source would have taken backups away from those installations entirely, and the sharing between two names is a disk layout detail rather than something the installation depends on. Losing it across a restore costs nothing the launcher promises.

tar supports it, though not under a name that says so. In write-entry.js the [FILE] path consults linkCache by dev:ino whenever nlink is above one, and only writes a Link entry on a hit. onWriteEntry fires inside [HEADER], after that decision has already been made, so there is no intervening from there, and 7.5.22 has no noHardlinks flag. What does work is passing a linkCache whose get always answers undefined: every name then goes down the ordinary file path. The deferral in pack.js that holds pending link jobs back is keyed off stat.nlink and released in [JOBDONE], not off the cache, so a cache that never answers does not strand a job.

What the archive holds now, listed off a real one. The source is Mods/carrycapacity.zip with a second name Mods/carrycapacity-1.0.0.zip over the same inode, nlink 2, sixteen bytes of content.

Before: Directory Mods/, File Mods/carrycapacity-1.0.0.zip size 16, Link Mods/carrycapacity.zip size 0 linkpath Mods/carrycapacity-1.0.0.zip

After: Directory Mods/, File Mods/carrycapacity-1.0.0.zip size 16, File Mods/carrycapacity.zip size 16

Both names carry their own bytes. After a restore the two files have nlink 1 and different inodes, which is also what validateTree wants: it refuses a hard link in an extracted tree, so putting the sharing back would have been refused at the far end regardless. One side effect worth naming: assertSafeCompressionTree already counted both names toward the total, and the archive now genuinely holds both, so the size cap and the progress figure describe what actually gets written rather than overstating it.

The regression test sits in the backup round trip block in tests/ipc/extraction.test.ts. It builds the pair with fs.link, asserts the two names really do share an inode before anything else happens, compresses, lists the archive and asserts that no entry is anything other than File or Directory and that both mod names carry their sixteen bytes, then restores and compares the tree byte for byte against the original and checks the two restored files are separate inodes.

Mutation, with the linkCache line taken back out: the compression still resolves, the archive comes back holding the Link entry above, and runExtraction on that same archive rejects. So the state being guarded against really is a backup that reported success and cannot be put back, not merely a different archive layout. The regression test fails on the entry type assertion in that state and passes again with the line restored.

Gates on Linux with Node 22: typecheck clean, lint:ci at 0 errors and the same 15 existing warnings, format:check clean, test:coverage at 137 files, 1649 passed and 2 skipped, 92.7 statements, 89.95 branches, 92.17 functions, 94.13 lines, every floor clear.

Not checked here: Windows, where NTFS has hard links too but the suite does not run.